-
Notifications
You must be signed in to change notification settings - Fork 752
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Re-introduce Microsoft.Extensions.ObjectPool.DependencyInjection #4038
Conversation
This is accounting for the aspnetcore api-review (method names, namespaces, api) |
Once this is merged please make an update in https://github.com/dotnet/api-catalog-infra/ - either update https://github.com/dotnet/api-catalog-infra/pull/59 (if it's still unmerged) or apply the renaming separately. |
...l.DependencyInjection.Tests/Microsoft.Extensions.ObjectPool.DependencyInjection.Tests.csproj
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.ObjectPool.DependencyInjection/PoolOptions.cs
Outdated
Show resolved
Hide resolved
...Microsoft.Extensions.ObjectPool.DependencyInjection/ObjectPoolServiceCollectionExtensions.cs
Outdated
Show resolved
Hide resolved
...Microsoft.Extensions.ObjectPool.DependencyInjection/ObjectPoolServiceCollectionExtensions.cs
Show resolved
Hide resolved
/// </summary> | ||
[Experimental] | ||
public static class ObjectPoolServiceCollectionExtensions | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we get back the overload that consumes a configuration section? We want to systematically enable things to be configured in this way and it's the convention in this repo.
Can we call all the configuration delegate just "configure" which is the convention we use in this repo?
Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently looking into why the section was removed after aspnetcore api-review, they had some arguments that might be worth checking.
configureOptions
: there are a few occurrences in this repos in case you think it's worth going through the rest of the code for consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@geeknoid This is like a custom deserialization for these settings. I see that it's checking the value is a valid integer, what else is it providing over the standard configuration methods?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So yes, we should change "configureOptions" to "configure" for general consistency.
Within this repo, we generally use a triplet of functions for configuring components. No options (you get the defaults), an Action delegate which lets you override options, and a ConfigurationSection to make it easy to consume the configuration state from an external JSON. This model has been working well for us.
@tekian Jan helped establish this pattern 3 years ago or so, maybe he has additional insights to provide.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With respect to configuration section, we make no assumptions about where in the configuration hierarchy is the section to configure the component. We let developer decide and hand us the section to bind to. IConfigurationSection
, as compared to IConfiguration
, knows Path
which was an information we wanted to capture to be able to reconstruct configuration schema.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thats fine but 90% of our code takes IConfiguration, not IConfigurationSection and it usually parses a known schema (more than just an int). The pattern should be consistent with the rest of the APIs in the stack.
...ies/Microsoft.Extensions.Http.Resilience/Hedging/Routing/RoutingStrategyBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
...Microsoft.Extensions.ObjectPool.DependencyInjection/ObjectPoolServiceCollectionExtensions.cs
Outdated
Show resolved
Hide resolved
...Microsoft.Extensions.ObjectPool.DependencyInjection/ObjectPoolServiceCollectionExtensions.cs
Outdated
Show resolved
Hide resolved
# Conflicts: # src/ToBeMoved/DependencyInjection.Pools/DependencyInjectionExtensions.cs
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
using System.Diagnostics.CodeAnalysis; | ||
|
||
namespace Microsoft.Extensions.DependencyInjection.Pools; | ||
namespace Microsoft.Extensions.ObjectPool; | ||
|
||
/// <summary> | ||
/// Contains configuration for pools. | ||
/// </summary> | ||
[Experimental] | ||
public sealed class PoolOptions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that this is now in the general-purpose Microsoft.Extensions.ObjectPool namespace, perhaps we should give this a more specialized name? ServiceCollectionPoolOptions maybe? Any other ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is hard, DependencyInjectionPoolOptions
? But the current name loks fine to me, there is no conflict and users will probably not use the class name directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why didn't this get called ObjectPoolOptions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Martin. It looks strange, that this type (or ObjectPoolOptions) is not used by Microsoft.Extensions.ObjectPool
|| Microsoft.Extensions.ObjectPool<T>
.
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
@RussKie flaky tests? also the Tests tab is empty on azdo, the one listing all failed tests and artifacts. |
I believe this is caused by #4078. I have a hunch Microsoft.Extensions.Diagnostics.ResourceMonitoring.Test could be unsettling the build agent - I saw a few build runs where tests from that project failed. |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
@sebastienros please don't forget about #4038 (comment). |
null
Microsoft Reviewers: Open in CodeFlow